feat: add MageForge Toolbar with basic audits#167
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new MageForge Toolbar UI on the storefront that hosts basic audit tools and integrates the Inspector activation button into the toolbar, alongside new backend configuration options for theme/label display and improved messaging.
Changes:
- Added a new frontend toolbar component (JS/CSS) with an audit registry and several initial WCAG/performance audits.
- Refactored the Inspector activation UI to be injected into the toolbar instead of using a standalone floating button.
- Added admin configuration for showing/hiding button labels and enabled the Inspector/Toolbar by default in config.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/view/frontend/web/js/toolbar.js | Registers Alpine toolbar component and manages lifecycle. |
| src/view/frontend/web/js/toolbar/ui.js | Builds toolbar DOM, audit menu UI, grouping, and outside-click handling. |
| src/view/frontend/web/js/toolbar/audits.js | Audit dispatcher (toggle/run/deactivate, group collapse, badge updates). |
| src/view/frontend/web/js/toolbar/audits/index.js | Audit registry and group definitions. |
| src/view/frontend/web/js/toolbar/audits/images-without-alt.js | Adds “images without alt” audit. |
| src/view/frontend/web/js/toolbar/audits/images-without-dimensions.js | Adds “images without width/height” (CLS) audit. |
| src/view/frontend/web/js/toolbar/audits/images-without-lazy-load.js | Adds “below-the-fold images without loading=lazy” audit. |
| src/view/frontend/web/js/toolbar/audits/inputs-without-label.js | Adds “inputs without accessible label” audit. |
| src/view/frontend/web/js/toolbar/audits/low-contrast-text.js | Adds “low contrast text” audit with contrast calculation. |
| src/view/frontend/web/js/toolbar/audits/tab-order.js | Adds “tab order visualization” audit with overlay + injected CSS. |
| src/view/frontend/web/js/inspector.js | Injects inspector button into toolbar and adds window-event plumbing/cleanup. |
| src/view/frontend/web/js/inspector/ui.js | Replaces floating-button state updates with toolbar event dispatch. |
| src/view/frontend/web/js/inspector/accessibility.js | Improves accessibility name resolution + focusability (hidden inputs). |
| src/view/frontend/web/css/toolbar.css | Adds full toolbar styling + shared highlight styles. |
| src/view/frontend/web/css/inspector.css | Adjusts inspector button styles to work inside toolbar layout. |
| src/view/frontend/web/css/audits/tab-order.css | Adds overlay styles injected by tab-order audit on demand. |
| src/view/frontend/templates/inspector.phtml | Loads toolbar assets and adds Alpine root node/config attributes. |
| src/Block/Inspector.php | Adds toolbar asset URLs + show_button_labels config accessor. |
| src/etc/adminhtml/system.xml | Adds/renames admin settings for toolbar + show button labels. |
| src/etc/config.xml | Enables inspector/toolbar by default and sets default show_button_labels. |
| src/Console/Command/Dev/InspectorCommand.php | Improves CLI messaging when inspector is disabled. |
src/view/frontend/web/js/toolbar/audits/inputs-without-label.js
Outdated
Show resolved
Hide resolved
| // Listen for inspector-state sync from toolbar | ||
| this._inspectorStateHandler = (e) => { | ||
| if (this._inspectorFloatButton) { | ||
| this._inspectorFloatButton.classList.toggle('mageforge-active', e.detail.active); | ||
| } | ||
| }; | ||
| window.addEventListener('mageforge:toolbar:inspector-state', this._inspectorStateHandler); |
There was a problem hiding this comment.
mageforge:toolbar:inspector-state is dispatched by the inspector (see updateFloatingButton()), but this file comments that it’s “sync from toolbar” and then listens for it just to toggle the inspector button class. Consider updating the button state directly (e.g., toggle on this._inspectorFloatButton) and only dispatching an event if another component truly needs it, or rename/relocate the event so the direction is clear.
| _appendInspectorButton() { | ||
| const toolbarContainer = document.querySelector('.mageforge-toolbar'); | ||
| if (toolbarContainer) { | ||
| this._inspectorFloatButton = this._createInspectorFloatButton(); | ||
| toolbarContainer.appendChild(this._inspectorFloatButton); | ||
| return; |
There was a problem hiding this comment.
_appendInspectorButton() always creates/appends a new .mageforge-inspector-float-button when it finds the toolbar. If the inspector Alpine component is initialised more than once (or reinitialised via navigation/partial updates), this can lead to duplicated Inspector buttons. Consider reusing an existing button if present, or removing any previous instance before appending a new one.
| /** | ||
| * Create floating button for inspector activation | ||
| */ | ||
| createFloatingButton() { | ||
| this.floatingButton = document.createElement('button'); | ||
| this.floatingButton.className = 'mageforge-inspector mageforge-inspector-float-button'; | ||
|
|
||
| // Propagate theme from root element to injected body element | ||
| if (this.$el && this.$el.hasAttribute('data-theme')) { | ||
| this.floatingButton.setAttribute('data-theme', this.$el.getAttribute('data-theme')); | ||
| } | ||
|
|
||
| this.floatingButton.type = 'button'; | ||
| this.floatingButton.title = 'Activate Inspector (Ctrl+Shift+I)'; | ||
| this.floatingButton.innerHTML = ` | ||
| <svg viewBox="0 0 16 16" xmlns="http://www.w3.org/2000/svg" fill="currentColor" height="20" width="20"> | ||
| <g stroke-width="0"></g> | ||
| <g stroke-linecap="round" stroke-linejoin="round"></g> | ||
| <g> | ||
| <path fill-rule="evenodd" clip-rule="evenodd" d="M1 3l1-1h12l1 1v6h-1V3H2v8h5v1H2l-1-1V3zm14.707 9.707L9 6v9.414l2.707-2.707h4zM10 13V8.414l3.293 3.293h-2L10 13z"></path> | ||
| </g> | ||
| </svg> | ||
| <span>MageForge Inspector</span> | ||
| `; | ||
|
|
||
| // Click to toggle inspector | ||
| this.floatingButton.onclick = (e) => { | ||
| e.preventDefault(); | ||
| e.stopPropagation(); | ||
| this.toggleInspector(); | ||
| }; | ||
|
|
||
| document.body.appendChild(this.floatingButton); | ||
| }, | ||
|
|
||
| /** | ||
| * Update floating button state | ||
| * Notify the toolbar about the current inspector open/closed state | ||
| */ | ||
| updateFloatingButton() { | ||
| if (!this.floatingButton) return; | ||
|
|
||
| if (this.isOpen) { | ||
| this.floatingButton.classList.add('mageforge-active'); | ||
| } else { | ||
| this.floatingButton.classList.remove('mageforge-active'); | ||
| } | ||
| window.dispatchEvent(new CustomEvent('mageforge:toolbar:inspector-state', { | ||
| detail: { active: this.isOpen } | ||
| })); | ||
| }, |
There was a problem hiding this comment.
updateFloatingButton() only dispatches a global event and doesn’t update the inspector button directly. Since the Inspector now owns _inspectorFloatButton, this could toggle the button’s active class directly and optionally dispatch a (clearly named) event for the toolbar to consume. Current naming/comment suggest the toolbar is being notified, but no toolbar listener exists in this PR.
| const _colorCanvas = document.createElement('canvas'); | ||
| _colorCanvas.width = _colorCanvas.height = 1; | ||
| const _colorCtx = _colorCanvas.getContext('2d', { willReadFrequently: true }); | ||
|
|
||
| /** | ||
| * Parse any CSS color string the browser understands into [r, g, b, a]. | ||
| * Uses Canvas to let the browser handle all color spaces (rgb, oklch, lab, etc.). | ||
| * | ||
| * @param {string} color | ||
| * @returns {number[]|null} [r, g, b, a] where a is 0–1, or null on failure | ||
| */ | ||
| function parseColor(color) { | ||
| if (!color || color === 'transparent') return null; | ||
| _colorCtx.clearRect(0, 0, 1, 1); | ||
| _colorCtx.fillStyle = '#fe01fe'; // sentinel: vivid pink never used as real text color | ||
| const sentinel = _colorCtx.fillStyle; // read back canonical form | ||
| _colorCtx.fillStyle = color; | ||
| // If fillStyle is unchanged, the browser could not parse the color | ||
| if (_colorCtx.fillStyle === sentinel) return null; | ||
| _colorCtx.fillRect(0, 0, 1, 1); | ||
| const d = _colorCtx.getImageData(0, 0, 1, 1).data; | ||
| return [d[0], d[1], d[2], d[3] / 255]; | ||
| } |
There was a problem hiding this comment.
This code assumes canvas.getContext('2d') always returns a valid context, but it can return null (e.g., if Canvas is unavailable/blocked), which would cause runtime errors in parseColor(). Add a guard for a null context and fail gracefully (e.g., skip the audit or treat colors as non-parseable).
| * Returns null if no parseable background is found (e.g. oklch/lab color spaces). | ||
| * | ||
| * @param {Element} el | ||
| * @returns {number[]|null} [r, g, b] or null |
There was a problem hiding this comment.
The effectiveBackground() docblock says it “Returns null if no parseable background is found”, but the implementation always returns an RGB array (defaulting to white when nothing is found). Please update the comment/@returns to match the actual behavior (or adjust the function to actually return null as documented).
| * Returns null if no parseable background is found (e.g. oklch/lab color spaces). | |
| * | |
| * @param {Element} el | |
| * @returns {number[]|null} [r, g, b] or null | |
| * Falls back to white when no parseable background is found. | |
| * | |
| * @param {Element} el | |
| * @returns {number[]} [r, g, b] |
| $value = $this->scopeConfig->getValue(self::XML_PATH_SHOW_BUTTON_LABELS); | ||
| // Default to true when not explicitly set to '0' | ||
| return $value !== '0'; |
There was a problem hiding this comment.
getShowButtonLabels() reads raw config via getValue() and compares strictly to the string '0'. Since getValue() returns mixed, non-string values (e.g. int 0/1) would make this behave incorrectly. Consider using isSetFlag() for this Yes/No config path (and rely on config.xml for the default).
| $value = $this->scopeConfig->getValue(self::XML_PATH_SHOW_BUTTON_LABELS); | |
| // Default to true when not explicitly set to '0' | |
| return $value !== '0'; | |
| return $this->scopeConfig->isSetFlag( | |
| self::XML_PATH_SHOW_BUTTON_LABELS | |
| ); |
| menuTitle.innerHTML = ` | ||
| <span class="mageforge-toolbar-menu-title-text">MageForge Toolbar</span> | ||
| <button type="button" class="mageforge-toolbar-menu-close" title="Close & deactivate all"> | ||
| <svg xmlns="http://www.w3.org/2000/svg" width="14" height="14" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2.5" stroke-linecap="round" stroke-linejoin="round"><path d="M18 6L6 18M6 6l12 12"></path></svg> | ||
| </button> | ||
| `; |
There was a problem hiding this comment.
The close button HTML uses title="Close & deactivate all" inside an innerHTML template. The & should be escaped (&) to keep the HTML valid and avoid entity parsing issues; alternatively set the title via setAttribute.
| const images = Array.from(document.querySelectorAll('img')).filter(img => { | ||
| if (!img.offsetParent && getComputedStyle(img).position !== 'fixed') return false; | ||
| const style = getComputedStyle(img); | ||
| if (style.visibility === 'hidden' || style.display === 'none' || style.opacity === '0') return false; |
There was a problem hiding this comment.
The audit’s visibility filter checks style.opacity === '0', but browsers may return '0.0'/'0.00'. This can include fully-transparent images in results; use a numeric check for opacity.
| if (style.visibility === 'hidden' || style.display === 'none' || style.opacity === '0') return false; | |
| const opacity = Number.parseFloat(style.opacity); | |
| if (style.visibility === 'hidden' || style.display === 'none' || opacity === 0) return false; |
| _createInspectorFloatButton() { | ||
| const btn = document.createElement('button'); | ||
| btn.className = 'mageforge-inspector-float-button'; | ||
| btn.type = 'button'; | ||
| btn.title = 'Activate Inspector (Ctrl+Shift+I)'; | ||
| btn.innerHTML = ` |
There was a problem hiding this comment.
The inspector button no longer gets a theme attribute / .mageforge-inspector wrapper class (it’s now just .mageforge-inspector-float-button). inspector.css theme overrides target .mageforge-inspector[data-theme="..."] .mageforge-inspector-float-button, so in light/auto mode the button won’t pick up the intended styling. Consider propagating data-theme to the button (or re-adding the mageforge-inspector class) or updating selectors to match the new toolbar embedding.
| const _colorCanvas = document.createElement('canvas'); | ||
| _colorCanvas.width = _colorCanvas.height = 1; | ||
| const _colorCtx = _colorCanvas.getContext('2d', { willReadFrequently: true }); | ||
|
|
There was a problem hiding this comment.
getContext('2d') can return null; _colorCtx is used unguarded (e.g. clearRect, fillStyle), which would throw and break the audit in that scenario. Add a null check and fail the audit gracefully (e.g. show an error badge) when the canvas context isn’t available.
| const images = Array.from(document.querySelectorAll('img')).filter(img => { | ||
| if (!img.offsetParent && getComputedStyle(img).position !== 'fixed') return false; | ||
| const style = getComputedStyle(img); | ||
| if (style.visibility === 'hidden' || style.display === 'none' || style.opacity === '0') return false; |
There was a problem hiding this comment.
Visibility check uses style.opacity === '0', which misses equivalent computed values like 0.0 and can produce false positives/negatives. Use a numeric check (e.g. parseFloat(style.opacity) === 0) for consistency with the lazy-load audit.
| if (style.visibility === 'hidden' || style.display === 'none' || style.opacity === '0') return false; | |
| if (style.visibility === 'hidden' || style.display === 'none' || parseFloat(style.opacity) === 0) return false; |
| border-radius: 10px; | ||
| box-shadow: 0 -8px 24px var(--mageforge-shadow-lg), 0 4px 8px var(--mageforge-shadow-sm); | ||
| padding: 6px; | ||
| min-width: 350px; |
There was a problem hiding this comment.
The audit menu is min-width: 350px, which will overflow on narrower viewports (many phones are <350px wide) and can cause horizontal scrolling/clipping. Consider using a responsive width (e.g. width/max-width based on 100vw) while keeping a comfortable desktop width.
| min-width: 350px; | |
| width: min(350px, calc(100vw - 16px)); | |
| max-width: calc(100vw - 16px); |
| width: 32px; | ||
| } |
There was a problem hiding this comment.
In the mobile breakpoint the burger button is forced to width: 32px but the label remains visible and padding/gap remain, so the content will likely overflow or be clipped. Either hide .mageforge-toolbar-burger-label at this breakpoint or adjust sizing/padding so the button can still accommodate the text.
| width: 32px; | |
| } | |
| width: 32px; | |
| padding: 0; | |
| gap: 0; | |
| } | |
| .mageforge-toolbar-burger-label { | |
| display: none; | |
| } |
This pull request introduces a new MageForge Toolbar component, refines the Inspector's activation button, and adds configuration options to improve usability and accessibility. The Inspector's floating button is now integrated into the Toolbar, and several configuration and UI improvements have been made in both the backend (admin settings) and frontend code.
Toolbar Integration and UI Refactoring:
src/view/frontend/web/js/inspector.js,src/view/frontend/web/js/inspector/ui.js,src/view/frontend/templates/inspector.phtml,src/view/frontend/web/css/inspector.css) [1] [2] [3] [4] [5] [6]Configuration and Admin UI Improvements:
src/etc/adminhtml/system.xml,src/etc/config.xml,src/Block/Inspector.php) [1] [2] [3] [4]Asset Management:
src/Block/Inspector.php,src/view/frontend/templates/inspector.phtml,src/view/frontend/web/css/audits/tab-order.css) [1] [2] [3]Accessibility and Usability Fixes:
aria-labelledbyreferences, excluding hidden inputs from focusability, and refining lazy loading status reporting. (src/view/frontend/web/js/inspector/accessibility.js) [1] [2] [3]Console Output Improvement:
src/Console/Command/Dev/InspectorCommand.php)